-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Don't capture breadcrumbs for Sentry XHRs #557
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
One downside of this integration test – it will attempt to request from http://example.com/api/1/store, but it fails because XHR from file:/// (where tests are served) cannot be initiated to http://. So no XHR actually takes place, but there is a warning message when running tests:
I'd stub this but ... the purpose of these integration tests is to be as true-to-life as possible, and should attempt actual XHRs. I wonder if I can just cancel/abort it in such a way that no attempt is made. |
src/raven.js
Outdated
}; | ||
|
||
// intentionally checking against 0 here (start of string) | ||
if (url.indexOf(self._globalEndpoint) !== 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This won't ever match since we make _globalEndpoint
protocol relative inside of _getGlobalServer
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or rather, I'm operating under the assumption here that the url for the http request got normalized and added the protocol. Maybe that's a bad assumption?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if we just did a substring match then? e.g. url.indexOf(self._globalEndpoint !== -1)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could also use parseUrl
and verify that host name and path match, omitting protocol.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What abotu checking for the public key anywhere int he url? That'd only exist in a url if it were bound for the sentry server since it should effectively be a UUID anyways.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a decent idea, but there's the possibility that somebody might somehow construct a URL to their own servers with the key ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean, the same argument can be said about the checking for the presence of _globalEndpoint
in a request bound for their servers.
If we want to be precise, we should validate the actual hostname, etc. It'd be marginally more expensive since doing this validation on every XHR request. So tradeoffs.
imo it's unlikely for someone to be kicking off requests to their own servers with the public key in the url, but it's your call.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mmm. Let's go with looking for the token. I'd rather keep the impact super minimal and stick with indexOf
if we can.
3880e00
to
5920f12
Compare
cc @mattrobenolt @mitsuhiko